The committers are authorized under a signed CLA.

• ✅ Steve Dodge (ea3274d)

If this is known to be incompatible with 9.0, maybe the whole subproject should use 9.1 in its name rather than 9.0?

If the instrumentation does not work correctly on 9.0, then it would be best if the type instrumentation is not applied there. This could be done by restricting it to 9.1+ by implementing InstrumentationModule#classLoaderMatcher. For example it seems that the class org.eclipse.jetty.client.api.ProxyConfiguration was removed in 9.1 and was not added back in later versions, so the classLoaderMatcher could make sure that class does not exist.

@agoallikmaa Thanks!

If the instrumentation does not work correctly on 9.0, then it would be best if the type instrumentation is not applied there. This could be done by restricting it to 9.1+ by implementing InstrumentationModule#classLoaderMatcher. For example it seems that the class org.eclipse.jetty.client.api.ProxyConfiguration was removed in 9.1 and was not added back in later versions, so the classLoaderMatcher could make sure that class does not exist.

I agree, with the 9.1 focus. Good find on the ProxyConfiguration class. Out of curiosity, is there a good approach or a tool that you could recommend to discover the class deltas from version to version? Or is it just stare-and-compare?

I agree, with the 9.1 focus. Good find on the ProxyConfiguration class. Out of curiosity, is there a good approach or a tool that you could recommend to discover the class deltas from version to version? Or is it just stare-and-compare?

I'm not aware of any tool for this, I have just been comparing versions manually.

@anuraaga - Regarding the non-agent usecase, I think I understand the ask, but need to clarify, is non-agent used when someone prefers to simply integrate the tracer code directly with their functional code? Please give me an example of how someone would use it on a non-functional basis. I think my code is close to enough that add a non-agent option wouldn't be much trouble as you explained, just want to make sure I have the usecase right.

@robododge The use case for library instrumentation is more about wanting to avoid the agent due to some extra overhead, (startup time overhead is unavoidable) and risk by such an invasive process. It's not to integrate tracer code directly into user business logic though, we provide something (interceptor, subclass, hooks, etc) that works with that library so the user can initialize it in their code, but still not really deal with tracing.

The unit test for Armeria library vs agent might be informative

Library:
https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/armeria-1.3/library/src/test/groovy/io/opentelemetry/instrumentation/armeria/v1_3/ArmeriaHttpClientTest.groovy#L14

Agent:
https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/armeria-1.3/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/armeria/v1_3/ArmeriaHttpClientTest.groovy#L15

All the unit tests are the same, the user just required an extra line to initialize using library instrumentation. So in this case, we would probably structure the test to let the library version return the tracing subclass.

Another request for help on this PR. I've been going through implementing all the changes as mentioned. All the comments here have steered me toward HttpClient.send() as the place to attach the tracer and also wrap callback listeners. But looks like we have another problem with API drift in the 9.x series. Jetty client 9.2 started using a different method signature for the send() method. But, from 9.2 on into 10.0, it seems to stay the same. So far I have cut out v9.0, now this would probably mean cutting out v9.2 as well. Any concerns?

• Send() - v9.1 protected void send(Request request, List<Response.ResponseListener> listeners)😟

• Send() - v9.2 protected void send(final HttpRequest request, List<Response.ResponseListener> listeners)

• Send() - v10.0.2 protected void send(final HttpRequest request, List<Response.ResponseListener> listeners)

My new technique is like this:

public class JettyClientTracing extends HttpClient {
 @Override
  protected void send(HttpRequest request, List<Response.ResponseListener> listeners) {
   //Setup the tracer, start span context and wrap all listeners
  }
}

from 9.2 on into 10.0, it seems to stay the same. So far I have cut out v9.0, now this would probably mean cutting out v9.2 as well. Any concerns?

hey @robododge! just confirming, you mean cutting out v9.1 as well? and keeping 9.2+?

@robododge Hmm that's an unfortunate change to an exposed method. Do you know if it's possible to implement both to support both versions? Something like

// 9.1
protected void send(Request request, List<Response.ResponseListener> listeners) {
  Context context = instrumenter.onStart(Context.current, request);
  listeners = decorateListeners(listeners, context);
  super.send(request, listeners);
}

// 9.2+
protected void send(HttpRequest request, List<Response.ResponseListener> listeners) {
  // Looks like our instrumentation can just handle Request
  Context context = instrumenter.onStart(Context.current, request);
  listeners = decorateListeners(listeners, context);
  super.send(request, listeners);
}

Not too much duplication

v9.1 is very old, I would vote to drop it if there's any added complexity to supporting it

(EDIT: v9.2.0 was released May 2014)

hey @robododge! just confirming, you mean cutting out v9.1 as well? and keeping 9.2+?

@trask - yes proposal right now is drop v9.0, v9.1

@robododge Hmm that's an unfortunate change to an exposed method. Do you know if it's possible to implement both to support both versions?

@anuraaga - the technique I'm using is to actually create a wrapper class that overrides the HttpClient.send(). The code won't compile for example when you you have an v9.2 style HttpRequest object and you try to call the v9.1 style send(Request request, List<Response.ResponseListener> listeners

@trask - yes proposal right now is drop v9.0, v9.1

@anuraaga thumbs'd up my comment above, so go for it, drop 'em 👍

Hi guys, are we good here? I want to get Jetty Http client 10 started after this

Hi guys, are we good here? I want to get Jetty Http client 10 started after this

I guess we're waiting for PR builds to come back